ref: Use undici to download binary #2993
Conversation
JPeer264
force-pushed
the
jp/upgrade-node-fetch
branch
from
b341615 to
99bef89
Compare
JPeer264
added
the
v3.0
JPeer264
force-pushed
the
jp/upgrade-node-fetch
branch
from
15cb52d to
0c9cd59
Compare
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| // Check if we have a total size to validate against | ||
| if (totalBytes > 0 && downloadedBytes < totalBytes) { | ||
| reject(new Error('connection interrupted')); | ||
| } else { | ||
| resolve(); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
CHANGELOG.md
Outdated
|
|
||
| ### Internal changes | ||
|
|
||
| - Removed `node-fetch` dependency when using the NPM package ([#2993](https://github.com/getsentry/sentry-cli/pull/2993)) |
There was a problem hiding this comment.
| - Removed `node-fetch` dependency when using the NPM package ([#2993](https://github.com/getsentry/sentry-cli/pull/2993)) | |
| - Removed `node-fetch` and `https-proxy-agent` dependencies when using the NPM package ([#2993](https://github.com/getsentry/sentry-cli/pull/2993)) |
CHANGELOG.md
Outdated
| ### Internal changes | ||
|
|
||
| - Removed `node-fetch` dependency when using the NPM package ([#2993](https://github.com/getsentry/sentry-cli/pull/2993)) |
There was a problem hiding this comment.
Since this is an internal change, we should not include it in the changelog imo.
| ### Internal changes | |
| - Removed `node-fetch` dependency when using the NPM package ([#2993](https://github.com/getsentry/sentry-cli/pull/2993)) |
I would also update the PR title from feat: to ref:, as it is not really a new feature from the user perspective. And, that way, the DangerJS action will not complain about a missing changelog entry, since changelog entries are not required for ref: PRs.
There was a problem hiding this comment.
I removed it and renamed the PR
JPeer264
changed the title
JPeer264
changed the title
| .on('finish', () => { | ||
| // Check if we have a total size to validate against | ||
| if (totalBytes > 0 && downloadedBytes < totalBytes) { | ||
| reject(new Error('connection interrupted')); | ||
| } else { | ||
| resolve(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Bug: Incomplete downloads of compressed files are silently accepted due to Content-Length unreliability and flawed totalBytes fallback logic.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
When downloading compressed HTTP responses, undici auto-decompresses, making the Content-Length header unreliable or unavailable. The code then defaults totalBytes to 0. If the connection is interrupted mid-download, the finish event still fires, but the condition totalBytes > 0 && downloadedBytes < totalBytes evaluates to false because totalBytes is 0. This causes the promise to resolve successfully, silently accepting a corrupted or incomplete binary file instead of rejecting with a "connection interrupted" error.
💡 Suggested Fix
Implement a robust mechanism to verify download completeness when Content-Length is unavailable, possibly by comparing downloadedBytes with an expected size or using a different stream completion check.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: scripts/install.js#L277-L284
Potential issue: When downloading compressed HTTP responses, `undici` auto-decompresses,
making the `Content-Length` header unreliable or unavailable. The code then defaults
`totalBytes` to 0. If the connection is interrupted mid-download, the `finish` event
still fires, but the condition `totalBytes > 0 && downloadedBytes < totalBytes`
evaluates to `false` because `totalBytes` is 0. This causes the promise to resolve
successfully, silently accepting a corrupted or incomplete binary file instead of
rejecting with a "connection interrupted" error.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5519175
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // Note: content-length might not be available if response was compressed, | ||
| // as native fetch decompresses transparently | ||
| const contentLength = response.headers.get('content-length'); | ||
| const totalBytes = contentLength ? parseInt(contentLength, 10) : 0; |
There was a problem hiding this comment.
Bug: Missing content-length causes division by zero in progress bar
When content-length header is missing (which can happen with transparent decompression, as noted in the comment), totalBytes becomes 0. The old code used parseInt(null, 10) which returned NaN, triggering the incorrectTotal check to return a no-op progress bar. The new code explicitly sets totalBytes to 0, which bypasses the incorrectTotal check (since 0 is a valid number). In the non-TTY case, this causes division by zero in Math.round((current / total) * 100), resulting in printing "Infinity%" to the log stream.
This removes
node-fetchentirely. We could have upgrade tonode-fetch@3, but this one requires ESM-only, which we can't support (we still need CJS support for our@sentry/*bundler packages).With this PR we use now
undiciwhich also has aProxyAgentincluded, so we got rid ofhttps-proxy-agentas well.Closes #1810
Closes CLI-25